Skip to content

[Fix]: Normalize Years and Months while constructing ISO8601 Duration String#2484

Merged
VardhanThigle merged 1 commit intoGoogleCloudPlatform:mainfrom
sagarwaal:use_normalize_string_for_interval
Jul 3, 2025
Merged

[Fix]: Normalize Years and Months while constructing ISO8601 Duration String#2484
VardhanThigle merged 1 commit intoGoogleCloudPlatform:mainfrom
sagarwaal:use_normalize_string_for_interval

Conversation

@sagarwaal
Copy link
Copy Markdown
Contributor

@sagarwaal sagarwaal commented Jun 26, 2025 •
edited
Loading

The change ensures that

  1. Years and months are normalized while constructing IS08601 duration string. For e.g. -
    • 1 year 13 Month -> P2Y1M (instead of P1Y13M)
    • 2 year 1 Month -> P2Y1M
  2. Interval of length 0 is always represented as P0Y
  3. Duration string for Interval with non-zero length doesn't have any specifier with 0 quantity, For e.g.
    • 1 hour -> PT1H (instead of P0DT1H)
    • 1 minute -> PT1M (instead of P0DT1M)

@sagarwaal sagarwaal requested a review from a team as a code owner June 26, 2025 05:28
@sagarwaal sagarwaal changed the title [Bug]: Normalize Years and Months while constructing ISO8601 Duration String [Fix]: Normalize Years and Months while constructing ISO8601 Duration String Jun 26, 2025
VardhanThigle
VardhanThigle reviewed Jun 26, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@VardhanThigle VardhanThigle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a ton for the change. If Google Sql needs a normalized entry, we should do that in dataflow.

I have triggered an IT flow, if any of the IT data needs to be tweaked for the normalisation, will let you know here.

@sagarwaal sagarwaal force-pushed the use_normalize_string_for_interval branch from a08ce97 to e893004 Compare June 26, 2025 09:22
VardhanThigle
VardhanThigle requested changes Jun 27, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@VardhanThigle VardhanThigle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might need to change
.isEqualTo(Value.string("P1000Y1000M3890DT30H31M12.000000009S")) at line 604 to a normazlied valaue too.

@sagarwaal sagarwaal force-pushed the use_normalize_string_for_interval branch from e893004 to 810d665 Compare June 27, 2025 05:36
@pull-request-size pull-request-size bot added size/L and removed size/S labels Jun 27, 2025
@sagarwaal sagarwaal force-pushed the use_normalize_string_for_interval branch from 810d665 to fce604a Compare June 30, 2025 08:02
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 2, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.50%. Comparing base (254a83e) to head (aceb80d).
Report is 12 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #2484      +/-   ##
============================================
+ Coverage     49.64%   54.50%   +4.86%     
+ Complexity     4807     1852    -2955     
============================================
  Files           941      445     -496     
  Lines         57683    24964   -32719     
  Branches       6233     2582    -3651     
============================================
- Hits          28635    13606   -15029     
+ Misses        27007    10542   -16465     
+ Partials       2041      816    -1225
Components Coverage Δ
spanner-templates 70.80% <100.00%> (+0.85%) ⬆️
spanner-import-export ∅ <ø> (∅)
spanner-live-forward-migration 78.79% <100.00%> (+0.01%) ⬆️
spanner-live-reverse-replication 77.37% <100.00%> (+0.01%) ⬆️
spanner-bulk-migration 87.89% <100.00%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
...er/migrations/avro/GenericRecordTypeConvertor.java 96.20% <100.00%> (+0.03%) ⬆️

... and 520 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sagarwaal sagarwaal force-pushed the use_normalize_string_for_interval branch from fce604a to aceb80d Compare July 2, 2025 13:34
@sagarwaal sagarwaal requested a review from VardhanThigle July 3, 2025 02:26
@VardhanThigle VardhanThigle merged commit 7b482c9 into GoogleCloudPlatform:main Jul 3, 2025
14 checks passed
MnkyGns pushed a commit to MnkyGns/DataflowTemplates that referenced this pull request Feb 12, 2026
@sagarwaal @MnkyGns
normalize years and months part in coverting Avro interval to ISO8601…
fdf9876 
… string (GoogleCloudPlatform#2484)

Co-authored-by: Sagar Agarwal <sagarwaal@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@VardhanThigle VardhanThigle VardhanThigle approved these changes

@pratickchokhani pratickchokhani Awaiting requested review from pratickchokhani pratickchokhani is a code owner automatically assigned from GoogleCloudPlatform/spanner-migrations-team

@asthamohta asthamohta Awaiting requested review from asthamohta asthamohta is a code owner automatically assigned from GoogleCloudPlatform/spanner-migrations-team

Assignees

No one assigned

Labels

bug-fix size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sagarwaal @VardhanThigle